Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Parse unnamed struct and union fields #99754

Closed
wants to merge 2 commits into from
Closed

Parse unnamed struct and union fields #99754

wants to merge 2 commits into from
+1,134 −118

Conversation

carbotaniuman
Copy link
Contributor

@carbotaniuman carbotaniuman commented Jul 26, 2022 •
edited
Loading

Reincorporates code from #84571 and #85515, for #49804 which was reverted due to context-sensitive issues around the union keyword.

This also incorporates a modified version of the error recovery from #88815, with the modifications being in where the parser recovery occurs.

The main issue I'm facing right now is in recovering for these types, as the current implementation fails to properly reject impl union {} for union {} {} without also incorrectly rejecting impl union {} and I'm not really sure how to resolve that without making the code a mess.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 26, 2022
@rust-highfive
Copy link
Collaborator

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Jul 26, 2022

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
calebcartwright
calebcartwright requested changes Jul 26, 2022
View reviewed changes
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review only applicable to the rustfmt subset of the diff

Thanks for including an implementation, but it's actually not necessary to do so and generally our preference in these new cases to leave the formatting untouched initially. We'll probably end up with something fairly similar on the rustfmt side, but we'll take care of that a little later on.

Inline comment left below on the one change requested for rustfmt, then believe the rustfmt changes in the other files could be reverted

src/tools/rustfmt/src/types.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member

Sorry for taking so long to get to this, not sure if I have enough context to review this.

r? compiler

fee1-dead
fee1-dead reviewed Aug 10, 2022
View reviewed changes
compiler/rustc_feature/src/active.rs Outdated
@@ -521,6 +521,8 @@ declare_features! (
/// Allows creation of instances of a struct by moving fields that have
/// not changed from prior instances of the same struct (RFC #2528)
(active, type_changing_struct_update, "1.58.0", Some(86555), None),
/// Allows unnamed fields of struct and union type
(incomplete, unnamed_fields, "1.53.0", Some(49804), None),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version number does not seem to be right..

src/test/ui/parser/issues/issue-88583-union-as-ident.rs Outdated Show resolved Hide resolved
@fee1-dead
Copy link
Member

I am not very familiar with ast code.

r? compiler

@rust-highfive rust-highfive assigned cjgillot and unassigned fee1-dead Aug 10, 2022
@cjgillot
Copy link
Contributor

If I understand the RFC correctly, we are trying to allow unnamed fields in struct & unions with either named or unnamed struct/union type. So an unnamed struct/union can only happen inside a _ field. You add unnamed struct & unions as type kinds. I'm not sure this is the best direction.
What about changing FieldDef using:

enum FieldDefKind {
    Named(Ident, P<Ty>),
    Unnamed(P<Ty>),
    Nested(StructOrUnion, Vec<FieldDef>)
}

This would probably remove a lot of the headaches about parsing ambiguities and having union {} pop up in unexpected places.
What do you think?

@carbotaniuman
Copy link
Contributor Author

That approach is #84415, and a comment was made suggesting the other approach. #84415 (comment)

Is the reasoning expressed in the comment still valid?

cjgillot
cjgillot reviewed Aug 14, 2022
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation @carbotaniuman. I left a few questions.

compiler/rustc_parse/src/parser/ty.rs
}
} else {
self.parse_path_start_ty(lo, allow_plus, ty_generics)?
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why in this order? Since we only allow anonymous types for recovery, should we start by parsing the path, and recover by parsing an anonymous type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this is the part of the change that I'm least sure of. In this code, because union can be an identifier, you have to expressly opt-in to parsing an anonymous union type. struct on the other hand is a keyword and will attempt to parse an anonymous struct type for recovery. This is really more of hack than anything proper.

This has some implications, such as impl struct {} having an error mentioning anonymous struct types (instead of expected type found keyword), but impl struct{} {} having a "proper" error. This may require recovery changes in the impl block parsing beyond what I have already done, but I'm not too sure on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carbotaniuman:

@Amanieu and I were reviewing this, and wondering if it would be possible to confine this entirely to field definitions, and even then only when the field name is _.

If the field name is _, then look for struct or union in the type. If the type is struct then it must continue on with {. If the type is union, it could be union; or union::something..., either of which would be the other case of unnamed fields (where you have a named type), but if it isn't the start of a path then it has to continue with {.

Either way, we're wondering if this can only be parsed in field types, and nowhere else, avoiding the need to handle it (for recovery or otherwise) in other locations a type can appear.

compiler/rustc_ast/src/visit.rs
@@ -456,6 +456,9 @@ pub fn walk_ty<'a, V: Visitor<'a>>(visitor: &mut V, typ: &'a Ty) {
TyKind::Typeof(ref expression) => visitor.visit_anon_const(expression),
TyKind::Infer | TyKind::ImplicitSelf | TyKind::Err => {}
TyKind::MacCall(ref mac) => visitor.visit_mac_call(mac),
TyKind::AnonymousStruct(ref fields, ..) | TyKind::AnonymousUnion(ref fields, ..) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explicitly ignore fields in visitors, so we have a compiler error when we change them.

Suggested change
TyKind::AnonymousStruct(ref fields, ..) | TyKind::AnonymousUnion(ref fields, ..) => {
TyKind::AnonymousStruct(ref fields, _recovered) | TyKind::AnonymousUnion(ref fields, _recovered) => {

compiler/rustc_ast/src/mut_visit.rs Outdated Show resolved Hide resolved
compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/item.rs Show resolved Hide resolved
compiler/rustc_parse/src/parser/item.rs
let (mut impl_items, err) =
self.parse_item_list(attrs, |p| p.parse_impl_item(ForceCollect::No))?;

if let Some(mut err) = err {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment explaining what happens here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhh this is with the other comment that I'm not too sure if this is the right way to do things, but basically I'm trying to parse an impl block, if that fails I try and parse an anonymous struct or union type and then parse an impl block. (Really I would need to do it three times to recover form impl union {}, but again I'm not even sure this is the right path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the error message really bad if you do not change the code for parse_item_list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's less that error message is bad and it's more that I can't do recovery (at least how I'm doing it) because calling the method during recovery will emit the error without allowing me to suppress it.

compiler/rustc_parse/src/parser/ty.rs Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Aug 14, 2022

☔ The latest upstream changes (presumably #100516) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2022
@JohnCSimon
Copy link
Member

Ping from triage:
@carbotaniuman - can you please post your status on this PR? Thank you

@carbotaniuman
Copy link
Contributor Author

carbotaniuman commented Oct 11, 2022 via email

@carbotaniuman
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot
Copy link
Collaborator

rustbot commented Nov 15, 2022

Some changes occurred in src/tools/cargo

cc @ehuss

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Nov 18, 2022

☔ The latest upstream changes (presumably #104330) made this pull request unmergeable. Please resolve the merge conflicts.

@apiraino
Copy link
Contributor

apiraino commented Nov 30, 2022 •
edited
Loading

Switching review status as at a glance I'm unsure if the latest commits replied in full to the previous review. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2022
@carbotaniuman
Copy link
Contributor Author

@rustbot ready

Mainly want a review on the general approach for error handling here, as I'm not too sure it's correct.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 7, 2022
@carbotaniuman carbotaniuman reopened this Jan 28, 2023
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-13 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:ac593985615ec2ede58e132d2e21d2b1cbd6127c)
Download action repository 'rust-lang/simpleinfra@master' (SHA:ece894d15649f0f9d7884f915dc821f00bd0418b)
Complete job name: PR (x86_64-gnu-llvm-13, false, ubuntu-20.04-xl)
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: x86_64-gnu-llvm-13
---

- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:4:11
-    |
- LL | fn f() -> struct { field: u8 } {}
-    |           ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:7:10
-    |
-    |
- LL | fn f2(a: struct { field: u8 } ) {}
-    |          ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:12:12
-    |
-    |
- LL |     field: struct { field: u8 }
-    |            ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:17:13
-    |
-    |
- LL |     field1: struct { field: u8 }
-    |             ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:21:10
-    |
-    |
- LL | struct I(struct { field: u8 }, u8);
-    |          ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:25:7
-    |
-    |
- LL |     K(struct { field: u8 }),
-    |       ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous fields are not allowed outside of structs or unions
-   --> $DIR/restrict_anonymous_structs.rs:28:9
-    |
-    |
- LL |         _ : struct { field: u8 }
-    |         -^^^^^^^^^^^^^^^^^^^^^^^
-    |         anonymous field declared here
- 
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:28:13
-   --> $DIR/restrict_anonymous_structs.rs:28:13
-    |
- LL |         _ : struct { field: u8 }
-    |             ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous fields are not allowed outside of structs or unions
-   --> $DIR/restrict_anonymous_structs.rs:33:9
-    |
- LL |         _ : u8
- LL |         _ : u8
-    |         -^^^^^
-    |         |
-    |         anonymous field declared here
- 
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:37:10
-    |
- LL | const L: struct { field: u8 } = 0;
-    |          ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:40:11
-    |
-    |
- LL | static M: struct { field: u8 } = 0;
-    |           ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:43:10
-    |
-    |
- LL | type N = struct { field: u8 };
-    |          ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
+ error: expected type, found keyword `struct`
78   --> $DIR/restrict_anonymous_structs.rs:46:6
79    |
79    |
80 LL | impl struct { field: u8 } {}
-    |      ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
+    |      ^^^^^^ expected type
82 
- error: anonymous structs are not allowed outside of unnamed struct or union fields
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:51:14
-    |
- LL | impl Foo for struct { field: u8 } {}
-    |              ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:55:13
-    |
-    |
- LL |     let p: [struct { field: u8 }; 1];
-    |             ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:58:13
-    |
-    |
- LL |     let q: (struct { field: u8 }, u8);
-    |             ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are not allowed outside of unnamed struct or union fields
-   --> $DIR/restrict_anonymous_structs.rs:61:19
-    |
-    |
- LL |     let c = || -> struct { field: u8 } {};
-    |                   ^^^^^^^^^^^^^^^^^^^^ anonymous struct declared here
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:4:11
-    |
-    |
- LL | fn f() -> struct { field: u8 } {}
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:7:10
-    |
-    |
- LL | fn f2(a: struct { field: u8 } ) {}
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:12:12
-    |
-    |
- LL |     field: struct { field: u8 }
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:17:13
-    |
-    |
- LL |     field1: struct { field: u8 }
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:21:10
-    |
-    |
- LL | struct I(struct { field: u8 }, u8);
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:25:7
-    |
-    |
- LL |     K(struct { field: u8 }),
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:28:13
-    |
-    |
- LL |         _ : struct { field: u8 }
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:37:10
-    |
-    |
- LL | const L: struct { field: u8 } = 0;
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:40:11
-    |
-    |
- LL | static M: struct { field: u8 } = 0;
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:43:10
-    |
-    |
- LL | type N = struct { field: u8 };
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:46:6
-    |
-    |
- LL | impl struct { field: u8 } {}
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:51:14
-    |
-    |
- LL | impl Foo for struct { field: u8 } {}
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:55:13
-    |
-    |
- LL |     let p: [struct { field: u8 }; 1];
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:58:13
-    |
-    |
- LL |     let q: (struct { field: u8 }, u8);
- 
- error: anonymous structs are unimplemented
-   --> $DIR/restrict_anonymous_structs.rs:61:19
-    |
-    |
- LL |     let c = || -> struct { field: u8 } {};
- 
- error: aborting due to 32 previous errors
+ error: aborting due to previous error
198 
---
To only update this specific test, also pass `--test-args unnamed-fields/restrict_anonymous_structs.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/unnamed-fields/restrict_anonymous_structs.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "--remap-path-prefix=/checkout/tests/ui=fake-test-src-base" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/unnamed-fields/restrict_anonymous_structs" "-A" "unused" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/unnamed-fields/restrict_anonymous_structs/auxiliary"
stdout: none
--- stderr -------------------------------
error: expected type, found keyword `struct`
  --> fake-test-src-base/unnamed-fields/restrict_anonymous_structs.rs:46:6
   |
LL | impl struct { field: u8 } {} //~ ERROR anonymous structs are not allowed outside of unnamed struct or union fields

error: aborting due to previous error
------------------------------------------

@bors
Copy link
Contributor

bors commented Feb 2, 2023

☔ The latest upstream changes (presumably #105670) made this pull request unmergeable. Please resolve the merge conflicts.

@WaffleLapkin
Copy link
Member

Marking as waiting on author again (merge conflicts and failing CI)

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2023
@tmandry
Copy link
Member

tmandry commented Apr 5, 2023

@carbotaniuman are you still working on this?

@cjgillot would you be willing to provide feedback on the error handling questions around #99754 (review)?

@joshtriplett
Copy link
Member

@carbotaniuman It looks like we're going to want this for the next version of libc. @Amanieu and I are really interested in what it would take to successfully ship this (both the parsing, and the subsequent implementation).

Is this something you're likely to continue working on in the near future?

@carbotaniuman
Copy link
Contributor Author

carbotaniuman commented Jun 2, 2023 via email

@joshtriplett
Copy link
Member

@carbotaniuman Ah, I see. So, by parsing the entire type, the parser then knows how much to skip over after saying that unnamed types aren't allowed in the context (rather than just encountering struct or union and failing)?

That makes sense.

@JohnCSimon
Copy link
Member

@carbotaniuman

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jul 30, 2023
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 30, 2023
@frank-king frank-king mentioned this pull request Aug 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fee1-dead fee1-dead fee1-dead left review comments

@cjgillot cjgillot cjgillot left review comments

@joshtriplett joshtriplett joshtriplett left review comments

@calebcartwright calebcartwright calebcartwright requested changes

Assignees

@cjgillot cjgillot

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.